Generate RPC calls in Bridge Client #1123
Conversation
|
|
There was a problem hiding this comment.
I think we might also want to generate all of that mess in temporalio/service.py. Off the top of my head, what I would suggest is to generate some file at temporalio/bridge/client_generated.py.
Could do the simple way of just having an init_workflow_service_client_calls(client: ServiceClient) that just does what it does today which is makes a _new_call for each known service name. Alternatively, a cleaner approach may be to just move the WorkflowService (and operator and cloud equivalents) to this generated file and have explicit methods created for each call that do as expected and then alias the service class publicly (or have the public WorkflowService class extend the internal/bridge form).
There was a problem hiding this comment.
IMO we should cargo fmt this and poe format the Python gen'd code upon generation
There was a problem hiding this comment.
Adding the cargo fmt will format the entire bridge source so will generate additional changes for the PR but it seems worth it to me.
temporalio/service.py
Outdated
| import temporalio.api.workflowservice.v1 | ||
| import temporalio.bridge.client | ||
| import temporalio.bridge.proto.health.v1 | ||
| import temporalio.bridge.services_generated as svc_gen |
There was a problem hiding this comment.
| import temporalio.bridge.services_generated as svc_gen | |
| import temporalio.bridge.services_generated |
Pedantic and not technically required, but usually we fully qualify each use when referring to ourselves in these files
There was a problem hiding this comment.
Updated these to be fully qualified. I also moved the generated python to be in bridge/generated so I could add a pydocstyle exclusion for the file. If that doesn't seem reasonable, I can see about spitting out some autogen'd doc strings.
There was a problem hiding this comment.
I do think that' be nice. Yeah, one issue about moving the base class into the bridge area is that it won't be as clear on https://python.temporal.io (we consider the bridge private code and don't show it in API docs). This will be the first time user-facing code references the bridge module (in this case for a class extension). But I don't think it's that big of a deal.
There was a problem hiding this comment.
Not huge, but I don't see any good reason to move the code to bridge rather than another generated file. Some autgenerated doc strings seem fine.
There was a problem hiding this comment.
Nothing blocking, though we may want to consider not putting user-facing components in internal bridge module. @tconley1428 - thoughts?
…ying on previous behavior but still contain the same assertions
…port in service.py
…s to a generated directory to allow pydocstyle exclusion. Add linter ignore rule on tests/worker/test_workflow.py:4840 to avoid a linter rule about method assignment.
VegetarianOrc
force-pushed
the
bridge/generate_rpc_calls
branch
from
a971dcf to
2812a80
Compare
| file_descriptors: list[FileDescriptor], | ||
| output_file: str = "temporalio/bridge/src/client_rpc_generated.rs", | ||
| ): | ||
| print("generating bridge rpc calls") |
There was a problem hiding this comment.
I think maybe one print for the whole thing, but I'm not sure we need all the steps. That said, marginal
| futures = "0.3" | ||
| prost = "0.13" | ||
| pyo3 = { version = "0.25", features = [ | ||
| "extension-module", |
There was a problem hiding this comment.
No good place to put this comment, but I think we should consider adding a test for the actual user issue, just to have the coverage we fixed it.
temporalio/service.py
Outdated
| import temporalio.api.workflowservice.v1 | ||
| import temporalio.bridge.client | ||
| import temporalio.bridge.proto.health.v1 | ||
| import temporalio.bridge.services_generated as svc_gen |
There was a problem hiding this comment.
Not huge, but I don't see any good reason to move the code to bridge rather than another generated file. Some autgenerated doc strings seem fine.
What was changed
This PR adds code generation of the RPC call wrapper in the bridge client to ensure ease the maintenance of manually updating the methods when core has a proto update. This generation has been added to the end of the
gen_protos_dockerscript.Some changes were included to make the generation possible:
rpc_callandrpc_call_on_traitmacros have been refactored to only include an exported macro namedrpc_callthat uses the fully qualified call syntax that was previously in the body ofrpc_call_on_trait.multiple-pymethodsfeature of PyO3 was enabled to allow theClientRefimpl to be split into two modules.clientmodule was elevated topub(crate)for referencing from theclient_rpc_generatedmodule.Why?
This allows us to easily keep the bridge client up to date with the RPC methods exposed via core.
Checklist
Closes [Bug] Batch operation feature in the Temporal Python SDK doesn't work #927
How was this tested:
The generated client was tested against the existing test suite and used to ensure the repro described in #927 correctly functions against a Temporal dev server.